Claude finished @zyl1121's task in 1m 48s —— View job

✅ Code Review — PR #455

Progress

• Gather context and understand the PR
• Read all changed files
• Analyze code for correctness, bugs, and edge cases
• Check test coverage and completeness
• Post review feedback

Overall Assessment

This is a clean, well-structured fix that correctly addresses the stale-node-health problem. The architecture is sound, the tests are thorough, and the prefilter hardening (Fatalf → Warnf + continue) is a significant reliability improvement.

Decision: APPROVE with minor observations — no blocking issues found.

Design Review

1. Clean separation of concerns (nodehealth package) ✅

The new pkg/base/nodehealth/health.go correctly encapsulates:

• Status struct combining Healthy + UnhealthyReason
• EvaluateFromFacts() — derives health from the four facts (reportedReady, heartbeatTime, now, timeout)
• ReadyConditionTrue() — extracts Kubernetes Ready condition

This makes the evaluation logic independently testable and reusable across localcache, nodemeta, and any future consumer.

2. Immutability on read paths ✅

All read paths now clone before mutating health state:

• localcache/export.go:cloneNodeWithCurrentHealth() — clones the node.Node, then applies health
• nodemeta/service.go:cloneSnapshotWithCurrentHealth() — same pattern for NodeSnapshot

Both TestGetNodeRefreshesCurrentHealthFromCachedFacts (export_test.go:270) and TestGetHealthyNodesByInstanceTypeFiltersExpiredHeartbeat (export_test.go:309) explicitly verify that cached source nodes are not mutated during reads. This prevents a class of subtle race-condition bugs.

3. Prefilter hardening ✅

The change from Fatalf to Warnf + continue in prefilter.go:77-94 is the most impactful reliability improvement. Previously, a stale metric or metadata update would crash the entire scheduler via Fatalf. Now it gracefully skips the stale node and continues. This alone prevents cascading failures when node heartbeats lag.

4. Proper JSON serialization ✅

• Healthy removed omitempty → now serializes "healthy":false explicitly (consumers can distinguish "unknown" from "unhealthy")
• UnhealthyReason with omitempty → only present when there's a reason
• ReportedReady with json:"-" → internal field, never serialized

This is validated by TestNodeSnapshotJSONIncludesHealthyFalseAndHidesReportedReady in service_resource_test.go:145.

Potential Issues & Observations

1. Duplicated timeout constant (minor)

metadataHealthTimeout() in export.go:166 and healthTimeout() in service.go:388 compute the same value: SyncMetaDataInterval + 10*time.Second. If one is updated independently, they'll drift. Consider extracting to a shared helper.

2. Lock scope in downNodeCache (very minor)

In node_cache.go:205-217, the lockMetaData mutex is released before delSortedNodes(n) is called. This was the case before the PR, so it's not a regression, but there's a tiny window between the unlock and the sorted-nodes deletion where a reader iterating sortedNodesByClusters could see a node that was just marked ReportedReady=false. In practice this is harmless since the next cycle will correct it.

3. Fast path removed in GetHealthyNodesByInstanceType

The PR removes the old fast path:

// old
if n == -1 {
    return clusterNodes
}

The old code returned raw references from the sorted cluster cache, which was efficient but allowed stale Healthy values to be used. The new code always clones and re-evaluates health, which is correct but has a performance cost on every call. This is the right trade-off — health correctness > micro-optimization.

4. appendSortedNodes reads n.Healthy without health recomputation

In node_cache.go:231, appendSortedNodes checks n.Healthy directly rather than recomputing it. This is fine for current callers because:

• From addNodeCache/constructNode: Healthy was just set from the DB live-status
• From updateNodeFromMetaData: Healthy was just set from n.Healthy on line 299
  But it means if someone calls addNodeCache with a node whose Healthy is out of date, the health-based filtering in appendSortedNodes will use that stale value. The node will still be in the cache, so subsequent reads through GetNodes, GetNode, etc. will re-evaluate correctly — it's only the sortedNodesByClusters membership that could lag.

Test Coverage

Missing (non-blocking): No test for GetNodes() or GetHealthyNodes() health refresh, though they use the same cloneNodeWithCurrentHealth path tested elsewhere.

Summary

The PR correctly fixes the stale heartbeat problem, is well-tested, and introduces no regressions. The design cleanly separates concerns, read paths are immutable, and the prefilter changes significantly improve scheduler robustness. No blocking issues found.

Claude finished @zyl1121's task in 2m 45s —— View job

Code Review — PR #455

Progress

• Analyze context and understand the request
• Read all changed files
• Analyze code for correctness, bugs, and edge cases
• Check test coverage and completeness
• Post review feedback

Overall Assessment

This PR correctly addresses the stale-node-health problem described in #454. The architecture is clean, the design is well-isolated, and tests are thorough. The Fatalf → Warnf + continue change in prefilter is a significant reliability improvement.

Decision: APPROVE — no blocking issues found. Observations below are minor and can be tracked separately.

Design Review

1. nodehealth package — clean abstraction ✅

pkg/base/nodehealth/health.go encapsulates the health evaluation logic:

• Status struct with Healthy + UnhealthyReason
• EvaluateFromFacts(reportedReady, heartbeatTime, now, timeout) — pure function from four inputs to one output
• Evaluate(conditions, heartbeatTime, now, timeout) — convenience wrapper that extracts ReadyConditionTrue

This is independently testable and reusable across localcache, nodemeta, and any future consumer. The two exported reason constants (ReasonReportedNotReady, ReasonHeartbeatExpired) make the API self-documenting.

2. Read-path immutability ✅

Both localcache and nodemeta now clone nodes before mutating health on read:

• localcache/export.go:170-179 — cloneNodeWithCurrentHealth() clones via n.Clone(), then applies health evaluation from cached facts
• nodemeta/service.go:488-492 — cloneSnapshotWithCurrentHealth() clones via cloneSnapshot(), then applies applyCurrentHealth()

All read paths (GetNodes, GetNode, GetNodesByIp, GetHealthyNodesByInstanceType, GetNode in nodemeta, ListNodes) use these helpers. Tests at export_test.go:296-306 and service_resource_test.go:121-143 explicitly verify source nodes are not mutated during reads.

3. Prefilter hardening ✅

In prefilter.go:78-94, the change from Fatalf to Warnf + continue for metadata and metric timeouts means stale nodes no longer crash the scheduler. This was the most impactful reliability win.

The Fatalf at line 73-77 for CpuLoadUsage > CpuTotal is correctly left unchanged — that condition represents a data integrity issue that warrants a crash.

4. JSON serialization ✅

node.go changes:

• Healthy removed omitempty → "healthy":false is now explicit in JSON
• UnhealthyReason keeps omitempty → only serialized when non-empty
• ReportedReady uses json:"-" → never serialized

The NodeSnapshot in nodemeta/service.go:98-118 mirrors these correctly.

Observations

1. metadataHealthTimeout() / healthTimeout() duplication

export.go:166-168 and service.go:388-390 both compute the same value: SyncMetaDataInterval + 10s. Both delegate to nodehealth.MetadataTimeout(), so the formula itself is shared — but they are separate wrapper functions. If the timeout derivation logic changes beyond the function call, only one wrapper might be updated. Minor, but worth tracking.

2. appendSortedNodes uses cached Healthy directly

At node_cache.go:230-233, appendSortedNodes gates on n.Healthy rather than recomputing health from facts:

func (l *local) appendSortedNodes(n *node.Node) {
    if n == nil || !n.Healthy {
        return
    }

This means:

• For nodes coming from toSchedulerNode (nodemeta path): Healthy was just computed from currentHealthStatus(), so it's correct.
• For nodes from constructNode (legacy DB path): Healthy equals constants.HeartbeatHealth == elem.LiveStatus, which is the persisted state rather than a freshness-based computation. A stale node loaded from DB with LiveStatus == HeartbeatHealth will be added to sortedNodesByClusters.

This is not a correctness bug because GetHealthyNodesByInstanceType recomputes health on every read via cloneNodeWithCurrentHealth() — so stale nodes in sortedNodesByClusters are filtered out during reads. The node appears in the sorted list but is excluded by the health filter. The impact is limited to a slightly larger sortedNodesByClusters list than necessary.

3. Redundant health checks in prefilter

prefilter.go:48-53 already skips !n.Healthy nodes. Then lines 90-95 check MetaDataUpdateAt timeout separately. Since Healthy is recomputed from MetaDataUpdateAt + timeout, the !n.Healthy check should catch stale-heartbeat nodes. The timeout check at line 90 acts as defense-in-depth in case a stale node somehow has Healthy=true (e.g., race with an update). The redundancy is acceptable.

4. downNodeCache mutates cached node in-place

node_cache.go:205-212 mutates the cached node directly with lockMetaData held, then releases the lock before calling delSortedNodes(n):

l.lockMetaData.Lock()
old := v.(*node.Node)
old.ReportedReady = false
old.Healthy = false
...
l.lockMetaData.Unlock()
l.delSortedNodes(n)

The mutation is correct (protected by lockMetaData), and delSortedNodes has its own lockSortedNodes lock. The brief window between Unlock and delSortedNodes is harmless — sortedNodesByClusters may include this node for an instant longer, but the next DB sync cleans it up. Not a regression.

5. incrementNodeCacheCounts in node_cache.go — consistent update

At node_cache.go:372, IncrNodeConcurrent calls getMutableNode(n), which fetches the cached node by n.ID(). Since read paths return a clone, IncrNodeConcurrent correctly routes to the cached reference. This means the clone/read pattern doesn't break concurrent counters.

Test Coverage

Coverage is thorough. Missing (non-blocking): no direct test for GetNodes() or GetHealthyNodes() health refresh, though both use the same cloneNodeWithCurrentHealth path.

Summary

The PR correctly fixes the stale-heartbeat problem with a clean architecture, immutable read paths, and comprehensive tests. The preloader reliability improvements (Fatalf → continue) are significant. No blocking issues.

Claude finished @zyl1121's task in 1m 1s —— View job

Re-review after commit 10fcd30 — Extra header comments removed

Progress

• Verify that extra header comments have been removed
• Confirm files are correct after the fix

✅ Fix verified

The follow-up commit 10fcd30 ("fix(CubeMaster): drop extra header comments") correctly addresses both comments from @fslongjin:

1. CubeMaster/pkg/base/nodehealth/health.go — now starts directly with package nodehealth (no blank copyright/SPDX header above the package declaration).

2. CubeMaster/pkg/selector/prefilter/prefilter_test.go — same fix applied, starts directly with package prefilter.

Both files now match the project's convention (as seen in prefilter.go:1-6 and export.go:1-6), where the copyright header comment sits above a package-level doc comment, not as a floating block on its own.

No other changes were detected in this push. The rest of the PR's code, tests, and design remain as reviewed in the previous round.

Decision: APPROVE — no issues remain.

Claude finished @zyl1121's task in 2m 20s —— View job

Claude is analyzing the latest PR state...

Progress

• Analyze all 5 inline review comments from @fslongjin
• Verify current code state against each concern
• Check if remaining fixes are needed
• Post analysis

Analysis of @fslongjin's Review Against Current PR State

Commit faba88d ("tighten stale-node health follow-ups") was pushed after @fslongjin's review and addresses all technical concerns. Here's the per-comment breakdown:

1. ✅ prefilter.go — Redundant MetaDataUpdateAt timeout check

Status: Fixed in faba88d

The redundant metaDataUpdateAtTimeout variable and the time.Since(n.MetaDataUpdateAt) > metaDataUpdateAtTimeout guard were both removed. The prefilter now relies solely on n.Healthy (line 47), which is already recomputed from heartbeat freshness by GetHealthyNodesByInstanceType → cloneNodeWithCurrentHealth → nodehealth.EvaluateFromFacts.

2. ✅ node.go:Clone() — Non-atomic read of LocalCreateNum

Status: Fixed in faba88d

Clone() now performs an atomic.LoadInt64(&n.LocalCreateNum) before the struct copy and overwrites the copied field, with an explanatory comment:

// Clone provides a best-effort read-side snapshot. Mutable counters such
// as LocalCreateNum are refreshed via atomic loads after the structural
// copy so cloned read models stay aligned with the write path.
localCreateNum := atomic.LoadInt64(&n.LocalCreateNum)
cloned := *n
cloned.LocalCreateNum = localCreateNum

3. ✅ service.go — Duplicate health evaluation

Status: Fixed in faba88d

toSchedulerNode no longer calls currentHealthStatus(snap, time.Now()). Instead it reads snap.Healthy and snap.UnhealthyReason directly — fields that were already set by applyCurrentHealth earlier in the call chain (UpdateNodeStatus line 240). There is now exactly one health evaluation per heartbeat update.

4. 📢 node.go:Healthy JSON tag — omitempty removed

Status: Noted, no code change needed

The omitempty removal is a deliberate wire-format change (confirmed by TestNodeSnapshotJSONIncludesHealthyFalseAndHidesReportedReady). As @fslongjin suggested, this should be called out in the PR description and next changelog entry — that's a documentation/communication task, not a code fix.

5. ✅ RegisterNode — Inconsistency with cloneSnapshotWithCurrentHealth

Status: Fixed in faba88d

Commit faba88d added applyCurrentHealth(snap, time.Now()) inside RegisterNode (line 197) before returning cloneSnapshot(snap) (line 200). This means the returned snapshot now has correct health (e.g., Healthy=false, UnhealthyReason="HeartbeatExpired" for a fresh node) rather than zero-value fields.

The function still returns cloneSnapshot(snap) rather than cloneSnapshotWithCurrentHealth(snap, time.Now()), but since applyCurrentHealth was just called, both produce identical results. This is consistent in practice.

Summary

All 5 inline review items from @fslongjin have been addressed:

No further code changes are needed. The PR is in good shape.

Thanks for this PR — the facts-vs-evaluation separation (ReportedReady vs Healthy) and the centralized nodehealth package are clean architectural improvements. I left a few inline comments on areas that could be tightened up.

Summary of inline comments

1. prefilter.go — Redundant MetaDataUpdateAt timeout check; GetHealthyNodesByInstanceType already expires stale heartbeats via cloneNodeWithCurrentHealth.
2. node.go:Clone() — Struct copy reads LocalCreateNum non-atomically (data race per Go memory model).
3. service.go — Duplicate health evaluation in UpdateNodeStatus (line 239) and toSchedulerNode (line 412); two time.Now() calls on the same snapshot.
4. node.go:Healthy JSON tag — omitempty removed; API contract change worth calling out explicitly.
5. service.go:RegisterNode — Returns cloneSnapshot without applying current health, inconsistent with GetNode/ListNodes/UpdateNodeStatus.

Thanks for the detailed review. I addressed the issues you found in follow-up commit, and I also updated the PR description to explicitly mention that healthy=false is now emitted in JSON output.

Hi @zyl1121, thanks for this PR!

The DCO check is currently failing because all 4 commits are missing the Signed-off-by trailer. The CubeSandbox project requires every commit to carry a DCO sign-off.

Could you please:

1. Amend each commit to add your Signed-off-by line (e.g., Signed-off-by: zhengyilei <zhengyilei@myhexin.com>)
2. Force push the updated branch

You can do this with an interactive rebase:

git rebase -i HEAD~4
# mark each commit as 'reword', and append:
# Signed-off-by: zhengyilei <zhengyilei@myhexin.com>

Or more conveniently use git rebase --signoff:

git rebase --signoff HEAD~4
git push --force-with-lease

Once all commits have the sign-off, the DCO check should pass. Thanks!

Claude finished @zyl1121's task in 1m 22s —— View job

PR #455 Re-trigger Analysis

Progress

• Gather context — check git state and new commits
• Analyze the request and determine what action to take
• Update GitHub comment with results

Current State

This is a re-trigger on a PR synchronize event. After examining the branch state:

✅ No new commits since last review

The same 4 commits reviewed previously are still present (latest: 7ebdb38, authored 2026-06-04 17:31:37 +0800). There are no new changes to review.

The PR is in good shape:

• All previous review feedback from @fslongjin was addressed in commit faba88d / 7ebdb38
• @fslongjin approved the PR on 2026-06-05
• Test coverage is thorough (unit tests for localcache, nodemeta, prefilter)
• The architecture (centralized nodehealth package, immutable read paths, prefilter hardening) is solid

⚠️ Remaining item: DCO sign-off

@fslongjin noted on 2026-06-05 that the DCO check is failing because all 4 commits are missing the Signed-off-by trailer. @zyl1121 (the author), you'll need to add the sign-off trailers. The easiest approach:

git rebase --signoff HEAD~4
git push --force-with-lease

This adds Signed-off-by: Your Name <your.email@example.com> to each commit. I cannot perform this step on your behalf, as the sign-off must come from the actual author.

Summary

Once the DCO sign-off is applied, this PR should be ready to merge.

Hi @zyl1121, thanks for this PR!

The DCO check is currently failing because all 4 commits are missing the Signed-off-by trailer. The CubeSandbox project requires every commit to carry a DCO sign-off.

Could you please:

1. Amend each commit to add your Signed-off-by line (e.g., Signed-off-by: zhengyilei <zhengyilei@myhexin.com>)
2. Force push the updated branch

You can do this with an interactive rebase:

git rebase -i HEAD~4
# mark each commit as 'reword', and append:
# Signed-off-by: zhengyilei <zhengyilei@myhexin.com>

Or more conveniently use git rebase --signoff:

git rebase --signoff HEAD~4
git push --force-with-lease

Once all commits have the sign-off, the DCO check should pass. Thanks!

Fixed and force-pushed the branch with signed-off commits. Thanks!

Claude finished @zyl1121's task in 34s —— View job

Progress

• Analyze PR synchronize event context
• Check for new commits on the branch
• Verify DCO sign-off status
• Update comment with findings

Analysis — PR #455 Synchronize Event

Current State

This is a re-trigger on a PR synchronize event. After examining the branch:

✅ No new commits since last review

The same 4 commits reviewed previously are still present (latest: 0aac550, all with DCO sign-off added). There are no new code changes to review.

✅ DCO sign-off confirmed

All 4 commits now carry the Signed-off-by: zhengyilei <zheng_yilei@qq.com> trailer:

3175c3e cubemaster: expire node health after stale nodemeta heartbeat
d1b27d4 cubemaster: centralize nodemeta heartbeat timeout
6c09b10 fix(CubeMaster): drop extra header comments
0aac550 fix(CubeMaster): tighten stale-node health follow-ups

The DCO check should now pass.

Summary

The PR is fully ready — all review feedback has been addressed, the DCO issue has been resolved, and the PR has been approved. No further action needed from my side.